[testharness.js] Disallow test return value
#12958
Conversation
The `test` function does not recognize the return value of the provided test body. Authors may mistakenly use this API instead of `promise_test`, a function which is designed to respond to promise values. Previously, mistakes like this would produce tests that spuriously passed. Update the `test` function to fail immediately in response to a body that returns a value. Also correct a test which exhibited the programming mistake that this change is intended to discourage (a survey using both the Chrome and Firefox browsers indicated that this is the only test in WPT which would be affected by this change).
wpt-pr-bot
requested review from
birtles,
flackr,
graouts,
gsnedders,
jgraham and
stephenmcgruer
|
Almost forgot: here's the branch I used to conduct the survey https://github.com/bocoup/wpt/commits/suspicious-sync |
resources/testharness.js
Outdated
| var value = test_obj.step(func, test_obj, test_obj); | ||
|
|
||
| test_obj.step(function() { | ||
| assert(value === undefined, |
There was a problem hiding this comment.
This is OK, but if I was being picky I might do
var msg = "Test object returned a value " + value";
if value.hasOwnProperty("then") {
msg += " Consider writing promise_test instead";
}
test_obj.set_status(test_obj.ERROR, msg);
test_obj.phase = test_obj.phases.HAS_RESULT;
test_obj.done();
There was a problem hiding this comment.
+1 for @jgraham's suggestion.
|
Thanks for the review! @jgraham's example referenced a non-existent property, I've updated the patch to report via a harness error. In order to limit the number of test files, I'm using a different approach for managing the tests. I've also implemented the hint that @jgraham originally requested and included some extra code to protect from the wacky values that tests might return. The original commit message is no longer accurate, but I'd be happy to update it once this patch is approved. |
|
The test for this needs to verify that a harness error is reported, so it can't be expressed using testharness.js alone. When I first automated this test suite, I extended the manual tests with JSON that describes how the harness is expected to behave. This seemed like the lowest-friction way to get us up and running, even if it did require specialized test execution logic. Since then, I've recognized a few deficiencies with the approach. For one, the tests are difficult to author because the process involves manually writing JSON into an HTML file, because error reports are difficult to interpret, and because there is some ambiguity with how the unordered subtest results should be encoded in an array. Secondly, there can only be one harness error per file, so when you are writing many tests for harness errors (as in the case here), you are forced to express expectations across many files. These problems were particularly frustrating in gh-8748, so I experimented with this approach there--see |
jugglinmike
changed the title
test return valuetest return value
The change in question: web-platform-tests/wpt#12958 This change was done to flag possible test authoring mistakes. While most of the updates in this CL are just adding braces arrow functions, in set-root-scroller.html there was a genuine problem with the test. In assert_selection.js, to avoid updating many many test using `test(() => assert_selection(...), desc)`, change `assert_selection` to not have a return value and add `assert_selection_and_return_sample`. Bug: 892613 Change-Id: Idaee49e40a9d85f5dfb7124c35a0d864bdbf016f Reviewed-on: https://chromium-review.googlesource.com/c/1268341 Reviewed-by: David Bokan <bokan@chromium.org> Reviewed-by: Yoshifumi Inoue <yosin@chromium.org> Commit-Queue: Philip Jägenstedt <foolip@chromium.org> Cr-Commit-Position: refs/heads/master@{#597834}
As a follow up to gh-12898, I wondered about similar test authoring mistakes
that we could avoid from within testharness.js. Are there any synchronous tests
that return a value? This would generally be harmless, but it that value were a
thenable, then it could be an unintentional use of
testinstead ofpromise_test. TaskCluster again helped to answer the question:The
testfunction does not recognize the return value of the providedtest body. Authors may mistakenly use this API instead of
promise_test, a function which is designed to respond to promisevalues. Previously, mistakes like this would produce tests that
spuriously passed.
Update the
testfunction to fail immediately in response to a bodythat returns a value. Also correct a test which exhibited the
programming mistake that this change is intended to discourage (a survey
using both the Chrome and Firefox browsers indicated that this is the
only test in WPT which would be affected by this change).